Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initialize and update the ydoc path property #342

Merged
merged 6 commits into from
Sep 20, 2024

Conversation

brichet
Copy link
Contributor

@brichet brichet commented Aug 29, 2024

This PR fixes #341

NOTE:

In this implementation, the document is reloaded when the path change, as if the file content has changed.
Another approach could be to add a listener on the path only in the FileLoader, which only update the path property of the ydoc on change.

Copy link
Contributor

Binder 👈 Launch a Binder on branch brichet/jupyter_collaboration/ydoc_file_path

@brichet brichet added the bug Something isn't working label Aug 29, 2024
@krassowski
Copy link
Member

which only update the path property of the ydoc on change

I feel like that would be a better approach, as any reloading from disk is a risk of race condition in RTC context, right?

@davidbrochart
Copy link
Collaborator

Thanks @brichet.
I tried to rename an opened file out-of-band, and it seems to crash?

@brichet
Copy link
Contributor Author

brichet commented Aug 29, 2024

Thanks @brichet. I tried to rename an opened file out-of-band, and it seems to crash?

Yes, it doesn't fix the issue on renaming the file out-of-band. It works when renaming the file using jupyterlab.
The issue you mentioned is related to jupyter_server_fileid: jupyter-server/jupyter_server_fileid#80

@brichet
Copy link
Contributor Author

brichet commented Aug 29, 2024

which only update the path property of the ydoc on change

I feel like that would be a better approach, as any reloading from disk is a risk of race condition in RTC context, right?

Any opinion on how to do it ?

The 2 options I can see are:

  • adding new methods, something like path_observer() and path_unobserver()
  • adding an optional argument (Callable) to the current observe() method

@davidbrochart
Copy link
Collaborator

I think you can now remove the note in the top comment?

Copy link
Collaborator

@davidbrochart davidbrochart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @brichet.

@brichet
Copy link
Contributor Author

brichet commented Aug 30, 2024

Is it expected that the new test fails in the context of Test Minimal Versions and test Prerelease ?

@davidbrochart
Copy link
Collaborator

You should be able to reproduce with Test Minimal Versions's environment on you side?

@brichet
Copy link
Contributor Author

brichet commented Aug 30, 2024

You should be able to reproduce with Test Minimal Versions's environment on you side?

It works on my side with python 3.8

@davidbrochart
Copy link
Collaborator

I think it's not only Python, but all dependencies. You can probably use uv for installing minimal versions.

@brichet
Copy link
Contributor Author

brichet commented Aug 30, 2024

I think it's not only Python, but all dependencies. You can probably use uv for installing minimal versions.

Not sure about that, I checked the packages and they are similar to mine.
However, I wonder if there is not an issue with the collaboration python package versions:
Using cached jupyter_server_ydoc-1.0.0b1-py3-none-any.whl

It should be the jupyter_server_ydoc-1.0.0b3 if it was the current dev install, no ?

@krassowski
Copy link
Member

krassowski commented Aug 30, 2024

Huh:

dependencies = [
"jupyter-server-ydoc>=1.0.0b1",
"jupyter-collaboration-ui>=1.0.0b1",
"jupyter-docprovider>=1.0.0b1"

dependencies = ["jupyter_collaboration>=3.0.0b3", "jupyter_collaboration_ui>=1.0.0b3", "jupyter_docprovider>=1.0.0b3", "jupyter_server_ydoc>=1.0.0b3"]

I thought I fixed it with #335 but clearly it did not work as intended and instead of replacing the old ones just added a new line on top:

image

which means the last one gets used

@brichet brichet mentioned this pull request Aug 30, 2024
brichet added a commit to brichet/jupyter_collaboration that referenced this pull request Sep 19, 2024
@davidbrochart
Copy link
Collaborator

Maybe exclude scripts/bump_version.py for mypy in pyproject.toml?

[tool.mypy]
exclude=[
    "^binder/jupyter_config\\.py$",
    "^scripts/bump_version\\.py$",
    "/setup\\.py$",
]

@brichet
Copy link
Contributor Author

brichet commented Sep 19, 2024

Maybe exclude scripts/bump_version.py for mypy in pyproject.toml?

Thanks @davidbrochart.

Maybe we could exclude it, but do you know why it fails in this PR ? It seems to pass on recent ones...

@davidbrochart
Copy link
Collaborator

do you know why it fails in this PR ? It seems to pass on recent ones...

I don't know, I've seen that in other repos too.

@brichet
Copy link
Contributor Author

brichet commented Sep 19, 2024

And it seems there are some other issue after the release:
Using cached jupyter_collaboration_ui-1.0.0b3-py3-none-any.whl, it should be 1.0.0b6 I think.

I will first remove the test to be able to merge this one.

@davidbrochart
Copy link
Collaborator

It looks like check-links is still failing, but let's merge this one.

brichet added a commit to brichet/jupyter_collaboration that referenced this pull request Sep 20, 2024
@brichet
Copy link
Contributor Author

brichet commented Sep 20, 2024

It looks like check-links is still failing, but let's merge this one.

Yes, it is not related

@brichet brichet merged commit b68d3cd into jupyterlab:main Sep 20, 2024
19 of 20 checks passed
@brichet brichet deleted the ydoc_file_path branch September 20, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Populating the path property of the ydoc
3 participants